feat(Responsive): add component #1872

layershifter · 2017-07-18T08:28:08Z

Why?

This component is the idea picked from my production project. Breakpoint uses syntax from Grid's device visibility, but has two important differences:

it doesn't require Grid markup, so two close items can have a different visibility
it doesn't render children when they doesn't visible

Warning

Breakpoint adds a listener to window that can cause a performance issue, however it will be solved by #1733.

P.S. Better ideas about the component's name are welcome.

…React into feat/breakpoint

codecov-io · 2017-07-18T11:00:44Z

Codecov Report

Merging #1872 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1872      +/-   ##
==========================================
+ Coverage   99.76%   99.76%   +<.01%     
==========================================
  Files         148      149       +1     
  Lines        2561     2584      +23     
==========================================
+ Hits         2555     2578      +23     
  Misses          6        6

Impacted Files	Coverage Δ
src/addons/Responsive/Responsive.js	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f39631...8474bba. Read the comment docs.

layershifter · 2017-07-18T12:40:49Z

package.json

@@ -130,7 +130,7 @@
    "satisfied": "^1.1.0",
    "semantic-ui-css": "^2.2.11",
    "simulant": "^0.2.2",
-    "sinon": "^2.1.0",
+    "sinon": "^2.3.8",


Sinon should be updated to support value:

sandbox.stub().value()

levithomason · 2017-08-10T21:25:27Z

src/addons/Breakpoint/Breakpoint.js

+    const { only = '' } = this.props
+    const points = only.replace('large screen', 'largeScreen').split(' ')
+
+    return _.some(points, point => _.invoke(this, point))


Where does this.point get defined?

_.invoke(this, point))

It equals to this[point](), while point is an element of array that is splitted from only prop. The idea is to call breakpoint matchers (methods) until match.

I will prefix breakpoints with is, it will be easier to read

levithomason · 2017-08-10T21:25:51Z

src/addons/Breakpoint/Breakpoint.js

+  // Helpers
+  // ----------------------------------------
+
+  visible = () => {


Let's prefix bools isVisible

layershifter · 2017-08-10T21:36:09Z

Also need merge with master

…React into feat/breakpoint

…antic-UI-React into feat/breakpoint # Conflicts: # src/addons/Breakpoint/Breakpoint.d.ts # src/addons/Breakpoint/Breakpoint.js # src/addons/Breakpoint/index.d.ts # src/lib/customPropTypes.js # test/specs/addons/Breakpoint/Breakpoint-test.js

layershifter

@levithomason I've prefixed all methods that returns boolean with is. It will be easier to understand what it makes.

levithomason · 2017-08-20T16:55:56Z

Naming

Per the request for naming, I've renamed this component:

<Responsive breakpoints={} />

I think this makes sense as many devs should be familiar with Bootstrap's "responsive utilities", which are classes for the same purpose.

breakpoints

I'm not sure about this prop. It seems it would be most helpful to set this at the LESS/CSS level instead. If we did allow overriding a specific <Responsive /> element with different values, what about something like this instead?

<Responsive minWidth={400} />
<Responsive maxWidth={800} />

What is the use case for overriding the value of mobile, etc?

layershifter · 2017-08-20T19:18:07Z

Naming

Agree about Responsive.

breakpoints

I've add it because widths that SUI uses are variables and it can be possible, that someone override them in own theme. Thats why I think we allow to change used values.

…React into feat/breakpoint # Conflicts: # package.json

…antic-UI-React into feat/breakpoint # Conflicts: # test/specs/addons/Breakpoint/Breakpoint-test.js

levithomason · 2017-08-26T18:10:46Z

Oh, I see. I was mistaken in thinking the only='mobile' here was relying on a mobile className and a CSS media query for visibility. I see now that this is only mimicking the CSS visibility styles.

The only issue I see here is that users with custom CSS breakpoints in their theme would expect this component to respect them, but it won't. Imagine I set my theme to have a mobile breakpoint of 400px:

<Column only='mobile' />      // only <400px (uses my LESS/CSS value)
<Responsive only='mobile' />  // only <300px (uses defaultProps value)

I'm worried about adding complexity and configuration to the theming process. It is quite cumbersome already.

My next suggestion was going to be that we should add CSS support for these utilties, however, it looks like SUI core isn't fond of responsive visibility or responsive utilities. There is a design decision to not permit them. While I don't agree, chances of getting into CSS core are minimal.

I'm still not comfortable merging this while we have two components with only='device' which would render differently. We need some kind of way to ensure all components use the same values.

layershifter · 2017-08-26T20:27:04Z

You justly paid attention to these things, but unfortunately I see no other way to solve this problem.

I want to solve the problem with useless rendering, Grid renders all elements in contrast to this component. It's very usefull for navbars and other responsive things.

levithomason · 2017-08-28T00:23:35Z

What if we remove the only prop and go with min/max width props instead? Users can create the same values locally:

const mediaQueries = { tablet: { minWidth: 768, maxWidth: 992 }  }

<Responsive {...mediaQueries.tablet} />

I would be OK with this as it doesn't suggest to the user that there is any relation between Responsive's only prop and Grid's only prop.

layershifter · 2017-08-28T08:39:58Z

I will think about this, but <Responsive only='mobile' /> looks more intuitive.

levithomason · 2017-08-28T09:31:59Z

You're right, it is very intuitive. If it used the same CSS as the Grid it would be a no brainer. Since they will produce 2 different behaviors, I think it becomes unintuitive.

One hack I didn't want to mention was somehow creating a ui grid element and checking the styles. I would rather not go down that road, but it may get us to where we need to be.

…React into feat/breakpoint

layershifter

@levithomason I refactored it to minWidth and maxWidth and added breakpoints as static properties. Also, it now uses eventStack and RAF for resize handler.

levithomason · 2017-09-11T03:33:10Z

Cool, seems like a good middle ground 👍 Thanks much for the cooperative spirit.

levithomason · 2017-09-11T04:15:28Z

Released in semantic-ui-react@0.73.0

* feat(Breakpoint): add component * feat(Breakpoint): add component * feat(Breakpoint): add component * style(Breakpoint): prefix bools with "is" * refactor(Breakpoint): rename Responsive * style(mixed): fix lint issues * feat(Responsive): refactor component * revert(customPropTypes): reverned unnecessary changes

Alexander Fedyashov and others added 3 commits July 17, 2017 17:59

feat(Breakpoint): add component

8c56cb9

feat(Breakpoint): add component

c419b05

Merge branch 'master' of https://github.com/Semantic-Org/Semantic-UI-…

1a52e98

…React into feat/breakpoint

layershifter added enhancement ready for review labels Jul 18, 2017

layershifter force-pushed the feat/breakpoint branch from c11e5b3 to 4cbd612 Compare July 18, 2017 11:00

layershifter force-pushed the feat/breakpoint branch 4 times, most recently from 62abdd4 to 6e17efd Compare July 18, 2017 12:30

layershifter commented Jul 18, 2017

View reviewed changes

feat(Breakpoint): add component

8e3209c

layershifter force-pushed the feat/breakpoint branch from 6e17efd to 8e3209c Compare July 20, 2017 07:30

layershifter mentioned this pull request Aug 4, 2017

fix(Grid): Stacking doesn't appear to do anything #1932

Closed

levithomason reviewed Aug 10, 2017

View reviewed changes

layershifter added needs author feedback and removed ready for review labels Aug 10, 2017

Alexander Fedyashov added 3 commits August 14, 2017 10:59

Merge branch 'master' of https://github.com/Semantic-Org/Semantic-UI-…

a3c6df1

…React into feat/breakpoint

style(Breakpoint): prefix bools with "is"

3e90730

layershifter commented Aug 14, 2017

View reviewed changes

layershifter added ready for review and removed needs author feedback labels Aug 14, 2017

refactor(Breakpoint): rename Responsive

3247632

layershifter added the needs author feedback label Aug 21, 2017

layershifter removed the ready for review label Aug 21, 2017

layershifter changed the title ~~feat(Breakpoint): add component~~ feat(Responsive): add component Aug 21, 2017

Alexander Fedyashov added 3 commits August 21, 2017 11:45

Merge branch 'master' of https://github.com/Semantic-Org/Semantic-UI-…

c8be674

…React into feat/breakpoint # Conflicts: # package.json

style(mixed): fix lint issues

6d63ded

Merge branch 'feat/breakpoint' of https://github.com/Semantic-Org/Sem…

aad879e

…antic-UI-React into feat/breakpoint # Conflicts: # test/specs/addons/Breakpoint/Breakpoint-test.js

layershifter added ready for review and removed needs author feedback labels Aug 21, 2017

levithomason added needs author feedback and removed ready for review labels Aug 26, 2017

Alexander Fedyashov added 3 commits September 3, 2017 17:52

Merge branch 'master' of https://github.com/Semantic-Org/Semantic-UI-…

c9141fc

…React into feat/breakpoint

feat(Responsive): refactor component

8187235

revert(customPropTypes): reverned unnecessary changes

8474bba

layershifter added ready for review and removed needs author feedback labels Sep 3, 2017

layershifter commented Sep 3, 2017

View reviewed changes

levithomason merged commit 3a39d64 into master Sep 11, 2017

levithomason deleted the feat/breakpoint branch September 11, 2017 03:33

layershifter mentioned this pull request Sep 28, 2017

Responsive: add fireOnMount prop and utils #2133

Closed

layershifter mentioned this pull request Nov 19, 2018

Add / change responsive grid size #3279

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Responsive): add component #1872

feat(Responsive): add component #1872

layershifter commented Jul 18, 2017

codecov-io commented Jul 18, 2017 •

edited

Loading

layershifter Jul 18, 2017 •

edited

Loading

levithomason Aug 10, 2017

layershifter Aug 10, 2017

layershifter Aug 11, 2017

levithomason Aug 10, 2017

layershifter commented Aug 10, 2017

layershifter left a comment

levithomason commented Aug 20, 2017

layershifter commented Aug 20, 2017 •

edited

Loading

levithomason commented Aug 26, 2017 •

edited

Loading

layershifter commented Aug 26, 2017

levithomason commented Aug 28, 2017

layershifter commented Aug 28, 2017

levithomason commented Aug 28, 2017

layershifter left a comment

levithomason commented Sep 11, 2017

levithomason commented Sep 11, 2017

feat(Responsive): add component #1872

feat(Responsive): add component #1872

Conversation

layershifter commented Jul 18, 2017

Why?

Warning

codecov-io commented Jul 18, 2017 • edited Loading

Codecov Report

layershifter Jul 18, 2017 • edited Loading

Choose a reason for hiding this comment

levithomason Aug 10, 2017

Choose a reason for hiding this comment

layershifter Aug 10, 2017

Choose a reason for hiding this comment

layershifter Aug 11, 2017

Choose a reason for hiding this comment

levithomason Aug 10, 2017

Choose a reason for hiding this comment

layershifter commented Aug 10, 2017

layershifter left a comment

Choose a reason for hiding this comment

levithomason commented Aug 20, 2017

Naming

breakpoints

layershifter commented Aug 20, 2017 • edited Loading

Naming

breakpoints

levithomason commented Aug 26, 2017 • edited Loading

layershifter commented Aug 26, 2017

levithomason commented Aug 28, 2017

layershifter commented Aug 28, 2017

levithomason commented Aug 28, 2017

layershifter left a comment

Choose a reason for hiding this comment

levithomason commented Sep 11, 2017

levithomason commented Sep 11, 2017

codecov-io commented Jul 18, 2017 •

edited

Loading

layershifter Jul 18, 2017 •

edited

Loading

layershifter commented Aug 20, 2017 •

edited

Loading

levithomason commented Aug 26, 2017 •

edited

Loading